-
-
Notifications
You must be signed in to change notification settings - Fork 313
Define compound schema documents (#936) #977
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Weird, I interpreted that issue as being about something completely different - that is, what happens when there is a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Relequestual (and team), great catch on the annotations and $ref
issue.
I think simplifying this down to strictly bundling, and then doing a separate PR to talk about dereferencing, would make this a lot easier.
jsonschema-core.xml
Outdated
When a reference applicator is dereferenced as part of the bundling process, the Schema Resource | ||
MUST NOT be embedded by replacing the schema object from which it was referenced, or by wrapping | ||
in other applicator keywords. In stead, a bundled Schema Resource MUST be located in `$defs`, | ||
where the key is the `$id` of the Schema Resource, which MUST then be referenced using `$ref`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is easier if you just say that the original reference MUST be left unchanged (but could later be changed through dereferencing, which is defined separately).
jsonschema-core.xml
Outdated
to the Compound Schema Document as an instance. It is RECOMMENDED that an alternate | ||
validation process be provided in order to validate Compound Schema Documents |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be an alternate for compound schema documents, or just an alternate for schema documents (schemas against meta-schemas) in general? You should always be able to tell whether a given schema is compound, so if you know your instance is a schema that is enough.
We don't want to encourage two different meta-schema functions, because if someone validating a non-bundled schema with the "regular" meta-schema validation function, but then that schema got changed to a bundled schema, it would now require a code change to be handled correctly. We never want to require a code change to keep validation working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The consensus in the associated issue was that a compound document would be transport only, in that you can't validate it by applying a meta schema to it as an instance.
Check the issue for the reasoning and justification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Relequestual I think @handrews was just concerned that this could be misunderstood to mean that you need a separate process to validate a schema than you do to validate a compound schema. The process for validating schema documents is the process for validating compound schema documents.
I think the suggestion would be to do something like ...
It is RECOMMENDED that an alternate validation process be provided in order to validate
CompoundSchema Documents
The "Compound" part is implied because all schemas are potentially compound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Relequestual yes, what @jdesrosiers said.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, we are on the same page now. I don't disagree.
But, I'm now not sure about a few things. I'm saying the following having not reviewed the issue again (because this is a quick comment in a few mins spare time).
I thought in the issue we said it wouldn't be possible to identify a compound schema document vs a normal schema document, although that may have been in context of being an instance.
I'm going to HAVE to review the issue discussion again. It's fine =]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought in the issue we said it wouldn't be possible to identify a compound schema document vs a normal schema document, although that may have been in context of being an instance.
Yes, that was in the context of being an instance. It's not possible to identify an instance as a schema (compound or otherwise).
Why do the referenced resources have to have absolute URI identifiers in order to be bundled? In my (admittedly simple) implementation of bundling in a json-schema-using application, which uses |
@karenetheridge What if |
@jdesrosiers All the definitions being moved are also scanned for embedded Just because there may be edge cases some of the time that require an |
Sorry, what I wrote doesn't make sense. Let me try again. You're right, you can bundle schemas without Ideally, whether a schema is bundled or not, you should get the same output. When you rewrite references, your Also, when you need to embed a schema of a different draft, you need to include |
I boradly agree with @jdesrosiers regarding tracability. It should be EASY for people to work out which of the schemas they have bundled, back to the original files. The FHIR bundled schema is comprised of a LOT of schemas. A LOT.
I'm not sure I follow. |
@Relequestual I didn't mean anything controversial with that statement. I just mean that without {
"$schema": "http://json-schema.org/draft-04/schema#",
"type": "object",
"properties": {
"foo": { "$ref": "./schema/foo#/definitions/aaa" }
}
} {
"$id": "http://example.com/schema/foo",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"aaa": { "const": "example" }
}
} Using the method @karenetheridge described for bundling, you would get this ... {
"$schema": "http://json-schema.org/draft-04/schema#",
"type": "object",
"properties": {
"foo": { "$ref": "#/definitions/aaa" }
},
"definitions": {
"aaa": { "const": "example" }
}
} This schema will not work properly because {
"$schema": "http://json-schema.org/draft-04/schema#",
"type": "object",
"properties": {
"foo": { "$ref": "#/definitions/aaa" }
},
"definitions": {
"aaa": {
"$schema": "http://json-schem.org/draft-07/schema#",
"const": "example"
}
}
} But, you can only use {
"$schema": "http://json-schema.org/draft-04/schema#",
"type": "object",
"properties": {
"foo": { "$ref": "#/definitions/aaa" }
},
"definitions": {
"aaa": {
"$id": "???",
"$schema": "http://json-schem.org/draft-07/schema#",
"const": "example"
}
}
} But, the referenced schema has no |
Thanks for clarifying @jdesrosiers, crystal clear now. |
I've made a few changes based on suggestions. |
Ah, it's failing because I need to add an anchor first, then xref target that anchor. I'll fix tomorrow. |
34bb747
to
396ec66
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good progress.
I think in general we want to be clear about the use case (bundling) vs the mechanism (compound documents) vs the notion of "dereferencing" which is actually something that happens during evaluation, at least based on how we use it elsewhere.
The question of "how do I detect and handle a compound schema document" is somewhat separate from "why would I make a compound schema document." The "why" part is important to motivate this, but once you have a compound document it doesn't matter why it was created.
jsonschema-core.xml
Outdated
</t> | ||
<section title="Bundling"> | ||
<t> | ||
A Compound Schema Document is created by taking references (such as "$ref") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically you can just write a compound schema document from the start, so I think a more accurate statement would be something like "A Bundled Schema is a Compound Schema Document that is created..." (idk if I'm capitalizing the right things there but that can be sorted out before publication).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm making a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is supposed to explain how to bundle multiple schemas to form a compound schema document. Therefore I think this is OK, but I'll re-phrase to make it clearer.
jsonschema-core.xml
Outdated
within the referreing document. | ||
</t> | ||
<t> | ||
Each embedded JSON Schema Resource (and the document's root) MUST identify itself |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the document's root is only a SHOULD for $id
, although the embedded resources are indeed a MUST. We probably just want to talk about the embedded ones here as the document root is covered under $id
.
jsonschema-core.xml
Outdated
MUST NOT be embedded by replacing the schema object from which it was referenced, or by wrapping | ||
in other applicator keywords. Instead, a bundled Schema Resource MUST be located as a value of | ||
a "$defs" object at the containing schemas root. The key of the "$defs" for the now embedded | ||
Schema Resource MAY be the "$id" of the bundled schema or some other form of application defined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idk that this is a MAY thing.... is there really anything to enforce here beyond the obvious need for unique keys under $defs
? Is this more of a best practices thing that belongs elsewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
Define Compound Schema Document and associated concerns
Fix typo Co-authored-by: Karen Etheridge <[email protected]>
Fix typo Co-authored-by: Karen Etheridge <[email protected]>
…d document. The phrase has a specific meaning
…scluding external Schema Resources
…, to said section
…nt, as it is possible to identify embedded schema resources if you know it's a schema as opposed to just an instance.
…enes need to change, and the of an embedded shema resource is still used as the referene string for by-reference applicator keywords.
…pound Schema Document
$id requirements for document root are already covered, and shouldn't be overriden.
Previously 'dereferenced' was used incorrectly. Now "bundling" is clearly defined, and MUST and MUST NOT are split for clairty.
I've forced pushed but something has done gone wrong or I've done something wrong with the naming of the branches and I'm not seeing my changes =/ |
Accidentally deleted the wrong branch in github UI... |
I think this is looking good- Any of my remaining comments are more personal style sort of things so however y'all want to resolve them (or leave as is) should be fine. |
Co-authored-by: Karen Etheridge <[email protected]>
…id the need to alter URIs used for referencing
…a Compound Schema Document
@karenetheridge After your further review, I think this is good to merge. |
Define Compound Schema Document and associated concerns
Resolves #936